node: add graphviz to container image #31
Conversation
| apt-get install -y \ | ||
| graphviz \ | ||
| ; \ | ||
| rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
Why do we need to delete the package information?
There was a problem hiding this comment.
It is standard practice in Dockerfiles to clean up package information/caches related to packager tooling to create lean images. This is the standard approach recommended with apt-get.
There was a problem hiding this comment.
I gather the samples repo runs on all Node targets? part of me wonders if we should have a specific node target that has all the dependencies needed by the samples repo, especially if we gradually add more over time.
Not a blocker if this helps get us over the finish line soon though; what are your thoughts @fhinkel?
|
What do you mean by a "node target"? My reading is that you are suggesting a variant of the node images with anything specific to the samples repo? If so, graphviz alone doesn't justify that. However, there are some shell scripts, one of which is used by every sample tested as a sort of container entrypoint. ]Test efficiency would be increased by moving some of the common or non-conflicting steps into the Dockerfile, such as the top-level Referenced Shell Scripts: |
There was a problem hiding this comment.
talked with @grayside in chat, for the first few additional dependencies like this, I see no problem with adding to the base container, if we find this is happening often, we might want a strategy that lets specific samples add additional dependencies to their build process as needed.
This is a step toward GoogleCloudPlatform/nodejs-docs-samples#1436.
An alternative to adding graphviz to the container is to pursue container-centric unit testing for Cloud Run on node.js as a follow-up to #16